GDPR: pass geo based on special feature 1 opt-in #2122

bsardo · 2022-01-05T16:41:52Z

This PR fixes a GDPR bug and depends on prebid/go-gdpr#34.
We currently determine if geo information should be passed in a bid request based on whether special purpose 1 is set in the GVL for the vendor which is incorrect. Instead we should be looking at whether special feature 1 is set for the vendor. The main change is here in impl.go on line 137:
passGeo = vendorException || (consentMeta.SpecialFeatureOptIn(1) && (vendor.SpecialFeature(1) || weakVendorEnforcement))

Special purposes are defined in the TCF2 spec but they are not used by PBS. As a result, all references to special purpose 1 have been renamed to eliminate confusion.

SyntaxNode · 2022-01-05T16:52:17Z

config/config.go

@@ -287,7 +287,7 @@ type TCF2 struct {
 	Purpose8            TCF2Purpose             `mapstructure:"purpose8"`
 	Purpose9            TCF2Purpose             `mapstructure:"purpose9"`
 	Purpose10           TCF2Purpose             `mapstructure:"purpose10"`
-	SpecialPurpose1     TCF2Purpose             `mapstructure:"special_purpose1"`
+	SpecialFeature1     TCF2SpecialFeature      `mapstructure:"special_feature1"`


Interesting. We were using the wrong terminology?

It's not just a matter of the wrong terminology. We are looking at the wrong field in the GVL. The go-gdpr package provides a vendor.SpecialPurpose method that checks if a special purpose is defined in the GVL specialPurposes field for a vendor. We are currently calling this method but that isn't correct. Instead we should be looking at GVL specialFeatures for a vendor. I have a go-gdpr PR out that is currently under review that will add a new method that allows us to access the correct field for a given vendor.

That'a concerning. What's the difference between a special purpose and a special feature? The TCF folks could've done better in the naming there.

SyntaxNode · 2022-01-05T16:54:29Z

config/config.go

+	for v := 0; v < len(c.GDPR.TCF2.SpecialFeature1.VendorExceptions); v++ {
+		bidderName := c.GDPR.TCF2.SpecialFeature1.VendorExceptions[v]
+		c.GDPR.TCF2.SpecialFeature1.VendorExceptionMap[bidderName] = struct{}{}
+	}


Please verify this code has full test coverage for cases of none, one, and many (2). It's been a problem for us in the past.

guscarreon

Left a nitpick

guscarreon · 2022-01-12T20:41:09Z

config/config_test.go

+		} else {
+			assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.enforce"), tt.description)
+			assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description)
+		}


I find this to be a very comprehensive test case but: can we make one last assertion in the resulting Configuration object itself? I realize TestSpecialFeature1VendorExceptionMap does this to an extent but not when oldSpecialFeature1Config is set. Maybe we could add an v.Unmarshal() call at the end of this test case so we assert that cfg.GDPR.TCF2.SpecialFeature1 was correctly set even when gdpr.tcf2.special_purpose1 is set as in oldSpecialFeature1Config.

824 for _, tt := range tests { 825 v := viper.New() 826 v.SetConfigType("yaml") 827 v.ReadConfig(bytes.NewBuffer(tt.config)) 828 829 migrateConfigSpecialFeature1(v) 830 831 if len(tt.config) > 0 { 832 assert.Equal(t, tt.wantSpecialFeature1Enforce, v.Get("gdpr.tcf2.special_feature1.enforce").(bool), tt.description) 833 assert.Equal(t, tt.wantSpecialFeature1VendorExceptions, v.GetStringSlice("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description) 834 } else { 835 assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.enforce"), tt.description) 836 assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description) 837 } + + var c Configuration + err := v.Unmarshal(&c) + assert.NoError(t, err, tt.desc) + + // Assert expected values on the `cfg.GDPR.TCF2.SpecialFeature1` object itself + assert.Equal(t, tt.wantSpecialFeature1Enforce, cfg.GDPR.TCF2.SpecialFeature1.Enforce, tt.desc) + assert.ElementsMatch(t, tt.wantSpecialFeature1VendorExceptions, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptions, tt.desc) 838 } config/config_test.go

Or even better, a New(v) call so we can also assert the correctness of the cfg.GDPR.TCF2.SpecialFeature1.VendorExceptionMap

824 for _, tt := range tests { 825 v := viper.New() 826 v.SetConfigType("yaml") 827 v.ReadConfig(bytes.NewBuffer(tt.config)) 828 829 migrateConfigSpecialFeature1(v) 830 831 if len(tt.config) > 0 { 832 assert.Equal(t, tt.wantSpecialFeature1Enforce, v.Get("gdpr.tcf2.special_feature1.enforce").(bool), tt.description) 833 assert.Equal(t, tt.wantSpecialFeature1VendorExceptions, v.GetStringSlice("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description) 834 } else { 835 assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.enforce"), tt.description) 836 assert.Nil(t, v.Get("gdpr.tcf2.special_feature1.vendor_exceptions"), tt.description) 837 } + + cfg := New(v) + + // Assert expected values on the `cfg.GDPR.TCF2.SpecialFeature1` object itself + assert.Equal(t, tt.wantSpecialFeature1Enforce, cfg.GDPR.TCF2.SpecialFeature1.Enforce, tt.desc) + assert.ElementsMatch(t, tt.wantSpecialFeature1VendorExceptions, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptions, tt.desc) + assert.Equal(t, tt.wantSpecialFeature1VendorExceptionsMap, cfg.GDPR.TCF2.SpecialFeature1.VendorExceptionMap, tt.desc) 838 } config/config_test.go

Another option is to probably add an oldSpecialFeature1Config scenatio to TestSpecialFeature1VendorExceptionMap? What do you think?

@guscarreon and I spoke offline about this.

My line of thinking was that the coverage was there across three unit tests but it was hard to see:
TestMigrateConfigSpecialFeature1 —> verifies the deprecated special purpose config values are properly mapped to the new special feature config values
TestFullConfig —> verifies all of the special purpose struct fields, including the vendor exception map, are correctly populated based on what’s set in the special feature config values
TestSpecialFeature1VendorExceptionMap —> verifies the vendor exception map is created correctly no matter how many elements are in the vendor exception slice

I'm hesitant to make use of another function in the package to perform assertions in a pure unit test so instead of calling New() we agreed to call unmarshal as a happy medium to ensure what's written to the viper config in the migrate function is able to be unmarshaled into the struct.

bsardo · 2022-01-13T02:00:58Z

I took a closer look at the special purpose to special feature config migration logic added here in config/config.go with @mansinahar's suggestion in mind and realized that no update is needed here.

To recap, during our group review it was pointed out that the TCF2Purpose struct had an Enabled field, which was recently deprecated, and an EnforcePurpose field, which was recently added as the replacement for Enabled. Before this PR, this struct was being used for both general purposes and special purposes so, from just looking at the struct, it appeared as though hosts could set EnforcePurpose for a special purpose and would receive a deprecation warning when doing so. However, that isn't the case. The migration logic that was added to migrate the TCF2Purpose Enabled field to EnforcePurpose a few months ago only threw deprecation warnings and mapped the Enabled value to EnforcePurpose for general purposes so hosts would never have seen a warning for a special purpose (see migrateConfigTCF2PurposeEnabledFlags in config/config.go.).

As such, I don't think it makes sense to do anything with the EnforcePurpose field in this new special purpose to special feature migration logic. The field simply existed on an internal data structure because the struct was being used to represent two kinds of things; it was never set nor was host ever informed to set it.

mansinahar · 2022-01-13T17:39:55Z

Sounds good! Thanks for looking into it @bsardo and providing clarification.

guscarreon

Looks great. Thank you for addressing my feedback

VeronikaSolovei9

Looks good, very straight forward.

* 'master' of https://github.com/wwwyyy/prebid-server: GDPR: pass geo based on special feature 1 opt-in (prebid#2122) Adyoulike: Fix adapter errors if no content (prebid#2134) Update adyoulike.yaml (prebid#2131) Add ORTB 2.4 schain support (prebid#2108) Stored response fetcher (with new func for stored resp) (prebid#2099) Adot: Add OpenRTB macros resolution (prebid#2118) Rubicon adapter: accept accountId, siteId, zoneId as strings (prebid#2110) Operaads: support multiformat request (prebid#2121) Update go-gdpr to v1.11.0 (prebid#2125) Adgeneration: Update request to include device and app related info in headers and query params (prebid#2109) Adnuntius: Read device information from ortb Request (prebid#2112) New Adapter: apacdex and Remove Adapter: valueimpression (prebid#2097) New Adapter: Compass (prebid#2102) Orbidder: bidfloor currency handling (prebid#2093)

GDPR: pass geo based on special feature 1 opt-in

5e51ffa

bsardo marked this pull request as draft January 5, 2022 16:42

SyntaxNode reviewed Jan 5, 2022

View reviewed changes

bsardo added 2 commits January 6, 2022 14:18

Merge branch 'master' into PBS-955-special-feature

676d685

Add test to ensure special feature 1 exception map is created correctly

7387ee7

bsardo marked this pull request as ready for review January 6, 2022 21:06

mansinahar requested review from guscarreon and VeronikaSolovei9 January 10, 2022 18:16

mansinahar assigned VeronikaSolovei9 and guscarreon Jan 10, 2022

guscarreon reviewed Jan 12, 2022

View reviewed changes

Update migration test to unmarshal edited viper config and verify

2ff502c

guscarreon approved these changes Jan 13, 2022

View reviewed changes

VeronikaSolovei9 approved these changes Jan 14, 2022

View reviewed changes

VeronikaSolovei9 merged commit 6e49445 into prebid:master Jan 14, 2022

ramyferjaniadot pushed a commit to adotmob/prebid-server that referenced this pull request Feb 2, 2022

GDPR: pass geo based on special feature 1 opt-in (prebid#2122)

6d98b98

jorgeluisrocha pushed a commit to jwplayer/prebid-server that referenced this pull request Sep 28, 2022

GDPR: pass geo based on special feature 1 opt-in (prebid#2122)

17cd5fd

shunj-nb pushed a commit to ParticleMedia/prebid-server that referenced this pull request Nov 8, 2022

GDPR: pass geo based on special feature 1 opt-in (prebid#2122)

b74b224

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

GDPR: pass geo based on special feature 1 opt-in #2122

GDPR: pass geo based on special feature 1 opt-in #2122

bsardo commented Jan 5, 2022 •

edited

Loading

SyntaxNode Jan 5, 2022

bsardo Jan 5, 2022

SyntaxNode Jan 5, 2022

SyntaxNode Jan 5, 2022

bsardo Jan 6, 2022

guscarreon left a comment

guscarreon Jan 12, 2022

bsardo Jan 13, 2022

bsardo commented Jan 13, 2022 •

edited

Loading

mansinahar commented Jan 13, 2022

guscarreon left a comment

VeronikaSolovei9 left a comment

GDPR: pass geo based on special feature 1 opt-in #2122

GDPR: pass geo based on special feature 1 opt-in #2122

Conversation

bsardo commented Jan 5, 2022 • edited Loading

SyntaxNode Jan 5, 2022

Choose a reason for hiding this comment

bsardo Jan 5, 2022

Choose a reason for hiding this comment

SyntaxNode Jan 5, 2022

Choose a reason for hiding this comment

SyntaxNode Jan 5, 2022

Choose a reason for hiding this comment

bsardo Jan 6, 2022

Choose a reason for hiding this comment

guscarreon left a comment

Choose a reason for hiding this comment

guscarreon Jan 12, 2022

Choose a reason for hiding this comment

bsardo Jan 13, 2022

Choose a reason for hiding this comment

bsardo commented Jan 13, 2022 • edited Loading

mansinahar commented Jan 13, 2022

guscarreon left a comment

Choose a reason for hiding this comment

VeronikaSolovei9 left a comment

Choose a reason for hiding this comment

bsardo commented Jan 5, 2022 •

edited

Loading

bsardo commented Jan 13, 2022 •

edited

Loading